Skip to content

Conversation

@RajPrakash681
Copy link
Contributor

@RajPrakash681 RajPrakash681 commented Oct 29, 2025

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

This PR removes redundant type definitions from the form builder and uses types directly from @openmrs/esm-form-engine-lib.

Changes made:

  • Removed local type definitions for Schema, Page, Section, Question, and QuestionOptions
  • Updated all imports across the codebase to use types directly from @openmrs/esm-form-engine-lib
  • Created FormBuilderSchema interface that extends FormSchema to include the description property, which is specific to the form builder's needs
  • Updated src/types.ts to export FormBuilderSchema as Schema for backward compatibility

Screenshots

N/A - No UI changes

Related Issue

https://issues.openmrs.org/browse/O3-1144

Other

None

…e-lib

- Import and re-export types (FormField, FormPage, FormSection, FormQuestionOptions, etc.) from @openmrs/esm-form-engine-lib instead of defining them locally
- Create type aliases (Page, Section, Question, QuestionOptions) pointing to form-engine types for backward compatibility
- Export additional useful types (QuestionAnswerOption, HideProps, DisableProps, RepeatOptions, etc.) from form-engine-lib
- Update add-form-reference.modal.tsx to use FormPage, FormSection, FormField instead of local Page, Section, Question types
- All tests passing, no breaking changes
@RajPrakash681
Copy link
Contributor Author

@NethmiRodrigo just got drained out in this issue...please let me know if these changes works

Copy link
Collaborator

@NethmiRodrigo NethmiRodrigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

translations?: Record<string, string>;
}
// Using extended FormBuilderSchema instead of FormSchema
export type Schema = FormBuilderSchema;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just directly import all types from the form-engine-lib itself and remove them from this file, we shouldn't need to export from here. Even though that would mean that we need to update a lot of files, it just seems redundant to export them from here again

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just directly import all types from the form-engine-lib itself and remove them from this file, we shouldn't need to export from here. Even though that would mean that we need to update a lot of files, it just seems redundant to export them from here again

You're absolutely right! Re-exporting these types is redundant. I'll update the codebase to import types directly from @openmrs/openmrs-form-engine-lib where they're used, rather than going through this intermediary file. This will make the dependencies clearer and reduce unnecessary abstraction.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we still re-exporting the schema from here? If it’s because the type from form-engine-lib is missing the description property, it’s highly possible that it's a mistake. Could you ask this in the #form-engine channel on Slack, and fix it in the form engine?

- Remove re-exported types from src/types.ts
- Update all files to import FormField, FormPage, FormSection directly from @openmrs/esm-form-engine-lib
- Keep only local types in src/types.ts (FormBuilderSchema, Form, Schema, Concept, etc.)
- This eliminates redundant type re-exports and makes dependencies clearer
@RajPrakash681
Copy link
Contributor Author

@NethmiRodrigo good morning ma'am any updates about it..?

@dkayiwa
Copy link
Member

dkayiwa commented Nov 2, 2025

ma'am

oh woow!!! 😊

@RajPrakash681
Copy link
Contributor Author

@NethmiRodrigo i think this issue's requirement is fulfilled please have a look and tell what more changes it needs!!

@RajPrakash681
Copy link
Contributor Author

@NethmiRodrigo this one also

translations?: Record<string, string>;
}
// Using extended FormBuilderSchema instead of FormSchema
export type Schema = FormBuilderSchema;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we still re-exporting the schema from here? If it’s because the type from form-engine-lib is missing the description property, it’s highly possible that it's a mistake. Could you ask this in the #form-engine channel on Slack, and fix it in the form engine?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please undo the changes to this file? And change your end of line as was mentioned in one of your other PRs in a different repo?

@NethmiRodrigo
Copy link
Collaborator

Also, I believe I’ve mentioned this before, please don’t change the PR template. Whenever you create a new PR, there will be a template, which you can fill out. Please stick to that and do not change it. AI tends to change it all the time, so make sure that you double check and change it back, this applies to all repositories, except where there isn’t a template.

@RajPrakash681
Copy link
Contributor Author

Also, I believe I’ve mentioned this before, please don’t change the PR template. Whenever you create a new PR, there will be a template, which you can fill out. Please stick to that and do not change it. AI tends to change it all the time, so make sure that you double check and change it back, this applies to all repositories, except where there isn’t a template.

Thanks for pointing that out, Nethmi. Usually, I fill out the PR template manually after you guided me about it earlier, but I missed it this time since I was in a bit of a hurry. Sorry about that I’ll make sure to double-check and stick to the template in all future PRs

@RajPrakash681
Copy link
Contributor Author

RajPrakash681 commented Nov 10, 2025

Why are we still re-exporting the schema from here? If it’s because the type from form-engine-lib is missing the description property, it’s highly possible that it's a mistake. Could you ask this in the #form-engine channel on Slack, and fix it in the form engine?

I'll ask about this in the #form-engine Slack channel and make the necessary changes based on their guidance. Will update you once I hear back.

@RajPrakash681
Copy link
Contributor Author

RajPrakash681 commented Nov 11, 2025

Why are we still re-exporting the schema from here? If it’s because the type from form-engine-lib is missing the description property, it’s highly possible that it's a mistake. Could you ask this in the #form-engine channel on Slack, and fix it in the form engine?

I'll ask about this in the #form-engine Slack channel and make the necessary changes based on their guidance. Will update you once I hear back.

The real question is whether or not the form-engine-lib needs to know about the description property on the FormSchema and I'm not entirely certain what it would do with that information. Thus, the thing the form-builder (which does use the description) is doing is probably right for now.

this is what ian said me when i asked him about this!

@RajPrakash681
Copy link
Contributor Author

RajPrakash681 commented Nov 17, 2025

@NethmiRodrigo hello, hope you would be doing well ! what should i do now in this.. any update ?

Copy link
Collaborator

@NethmiRodrigo NethmiRodrigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor nit

…dd-form-reference.modal.tsx

Co-authored-by: Nethmi Rodrigo <[email protected]>
@RajPrakash681
Copy link
Contributor Author

One minor nit

Done!

@RajPrakash681
Copy link
Contributor Author

@NethmiRodrigo Hi, can this be merged now or does this require more things to fix!?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants